Implement ACL attribute read/write #12305
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add empty cluster implementation to all-clusters-app example.
|
Converted to draft as dependent PRs still need to be merged first. |
mlepage-google
changed the title
|
Also note that only access-control-server.cpp changed (and its BUILD file), the rest is all what this should be rebased on top of. |
|
PR #12305: Size comparison from 8a2fd0d to 4561958 Increases above 0.2%:
Increases (2 builds for linux)
Full report (29 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
|
|
PR #12305: Size comparison from 77258f7 to b8a6bef Increases above 0.2%:
Increases (2 builds for linux)
Full report (29 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
|
For esp32 and mbed, for all-clusters-app.
Code gen isn't working yet for this possibility.
mlepage-google
force-pushed
the
access-control-server-implementation
branch
from
b8a6bef to
a51525c
Compare
|
PR #12305: Size comparison from f7f1cc3 to a51525c Increases above 0.2%:
Increases (2 builds for linux)
Decreases (13 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
Full report (29 builds for efr32, k32w, linux, nrfconnect, qpg, telink)
|
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller and
woody-apple
|
All checks passing. This PR will allow read and write of ACL list as a whole. |
andy31415
approved these changes
Dec 6, 2021
|
fast track: pr open for a long time, created by domain owner |
woody-apple
approved these changes
Dec 6, 2021
|
@mlepage-google Can you please re-gen/fix conflicts? |
|
@woody-apple yep I was going to merge/regen/restyle/etc. after review/approval. I'll do it shortly when I get a moment between other PRs. |
|
PR #12305: Size comparison from a1cb341 to 31853e4 Decreases (6 builds for efr32, k32w, qpg, telink)
Full report (11 builds for efr32, k32w, qpg, telink)
|
It was TagBoundEncoder, but now it's ListEncodeHelper. Also had to fix the perfect forwarding in AttributeValueEncoder and ListEncodeHelper and other template functions in the file, so it doesn't try to copy its args.
|
PR #12305: Size comparison from a1cb341 to 12b0651 Increases above 0.2%:
Increases (11 builds for esp32, k32w, mbed, nrfconnect, p6)
Decreases (14 builds for efr32, k32w, mbed, nrfconnect, p6, telink)
Full report (28 builds for efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
|
These static constexpr member variables were declared but not defined. In C++14 and lower, the definitions are required. Otherwise, undefined reference ensues. C++17 handles this but Matter is still using C++14. Not sure why this was working before...
|
PR #12305: Size comparison from a1cb341 to e19c1ad Increases (3 builds for k32w, qpg)
Decreases (3 builds for k32w, qpg, telink)
Full report (7 builds for k32w, qpg, telink)
|
|
PR #12305: Size comparison from 5c70530 to b7e997e Increases above 0.2%:
Increases (22 builds for esp32, k32w, linux, mbed, nrfconnect, p6, qpg)
Decreases (10 builds for efr32, mbed, p6, telink)
Full report (39 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
| @@ -979,12 +979,12 @@ | |||
| "mfgCode": null, | |||
| "side": "server", | |||
| "included": 1, | |||
| "storageOption": "External", | |||
| "storageOption": "RAM", | |||
There was a problem hiding this comment.
Why? This is going to take up space in the attr store that is completely unused...
There was a problem hiding this comment.
This is the easiest way to get the tests running for now, because the attribute exists. Otherwise I have to go through and ensure all the "not yet implemented" parts have "some kind of implementation" sufficient to keep the tests happy. I made the length 4 just in case any test tried to write more than one, but I could change the length to 1 for now.
There was a problem hiding this comment.
Please see PR #12690
|
|
||
| struct AccessControlEntryCodec | ||
| { | ||
| static CHIP_ERROR Convert(AuthMode from, AccessControlCluster::AuthMode & to) |
There was a problem hiding this comment.
This to me suggests that Access::AuthMode should be a type alias for AccessControlCluster::AuthMode, so all we need to do here is check that we're in range....
There was a problem hiding this comment.
That would make the code a tiny bit smaller (how much? a few small switch statements would go away). But eventually (when we get beyond features and into optimization) I want to bit encode the auth mode and privilege together. That will optimize for the common runtime check case, whereas managing ACL entries occurs much less often.
| return CHIP_NO_ERROR; | ||
| } | ||
|
|
||
| static CHIP_ERROR Convert(Privilege from, AccessControlCluster::Privilege & to) |
There was a problem hiding this comment.
Likewise, it feels like the code here could be much smaller/simpler if our types were aligned better.
| CHIP_ERROR err = aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { | ||
| while (iterator.Next(codec.entry) == CHIP_NO_ERROR) | ||
| { | ||
| encoder.Encode(codec); |
There was a problem hiding this comment.
Why are we ignoring failures here?
There was a problem hiding this comment.
Good point, I'll add checks.
There was a problem hiding this comment.
Please see PR #12690
|
|
||
| CHIP_ERROR AccessControlAttribute::ReadExtension(AttributeValueEncoder & aEncoder) | ||
| { | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
How about we explicitly encode an empty list and not bloat our RAM use?
There was a problem hiding this comment.
I can spin a PR for that, or change the XML length to 1 and reduce the octstr size, until we have a real implementation. Of the two temporary situations, which is preferable?
There was a problem hiding this comment.
Please see PR #12690
|
|
||
| CHIP_ERROR AccessControlAttribute::WriteExtension(AttributeValueDecoder & aDecoder) | ||
| { | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
This needs a TODO, right?
There was a problem hiding this comment.
We have issues, extensions are #10252. I find if I add TODOs everywhere in the skeleton code, TODObot gets very talkative. The issue should be sufficient? (It's returning no error, because I'd prefer "not implemented", but tests that try to read/write every attribute complain.)
Comment on lines
+425
to
+427
| AccessControlAttribute gAttribute; | ||
|
|
||
| AccessControl gAccessControl(Examples::GetAccessControlDelegate()); |
There was a problem hiding this comment.
Both of these should be static or inside an anonymous namespace. Preferably the latter, along with the various struct definitions above.
There was a problem hiding this comment.
I agree I'll do that.
There was a problem hiding this comment.
Please see PR #12690
|
@mlepage-google See above? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Access Control Cluster needs an implementation (issue #10254)
Change overview
Implement read and write of entire ACL attribute. (Not list indexes, not fabric indexing, not parts of entries...)
Includes temporary access control set up (which will be moved later).
Note that access control isn't persisted or hooked up to IM yet.
Testing
Tested by running chip-tool against chip-all-clusters-app to read and write ACL entries and inspect that operation is correct.